Repository navigation
Fixed issue where some outbound API requests dont have correct otel headers - #197
Conversation
WalkthroughIntroduces OpenTelemetry trace header injection in the API client’s request flow, updates tests to validate propagation behavior, performs a minor type assertion tweak in Vector API, and bumps version with a changelog entry. No exported API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant C as API Client
participant OT as OTEL Propagation
participant F as fetch()
participant S as Outbound Service
U->>C: send(request, headers)
alt Active OTEL context
C->>OT: propagation.inject(context.active(), carrier)
OT-->>C: trace headers set (e.g., traceparent)
else No active context
Note over C: No injection performed
end
C->>C: Merge existing headers + Authorization
C->>F: fetch(url, { headers })
F->>S: HTTP request with final headers
S-->>F: Response
F-->>C: Response
C-->>U: Result
note over C,S: Existing trace headers are preserved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/apis/vector.ts (1)
140-140: Minor type formatting adjustment.The semicolon removal inside the inner object type (
{ id: string; }→{ id: string }) is a stylistic change with no functional impact. This appears to be an unrelated cleanup that wasn't mentioned in the PR objectives or AI summary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonsrc/apis/prompt/generated/_index.jsis excluded by!**/generated/**
📒 Files selected for processing (5)
CHANGELOG.md(1 hunks)package.json(2 hunks)src/apis/api.ts(2 hunks)src/apis/vector.ts(1 hunks)test/apis/api.test.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
test/**
📄 CodeRabbit inference engine (AGENT.md)
Tests must mirror the source structure under the test/ directory
Files:
test/apis/api.test.ts
{src,test}/**/!(*.d).ts
📄 CodeRabbit inference engine (AGENT.md)
{src,test}/**/!(*.d).ts: Use strict TypeScript and prefer unknown over any
Use ESM import/export syntax; avoid CommonJS require/module.exports
Use relative imports for internal modules
Keep imports organized (sorted, no unused imports)
Use tabs with a visual width of 2 spaces
Limit lines to a maximum of 80 characters
Use single quotes for strings
Use proper Error types; do not throw strings
Prefer template literals over string concatenation
Files:
test/apis/api.test.tssrc/apis/api.tssrc/apis/vector.ts
src/apis/**
📄 CodeRabbit inference engine (AGENT.md)
Place core API implementations under src/apis/ (email, discord, keyvalue, vector, objectstore)
Files:
src/apis/api.tssrc/apis/vector.ts
src/apis/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/code-generation.mdc)
src/apis/**/*.ts: Do not hardcode generated prompt content (e.g., copyWriter) in source files; load it dynamically
Avoid overly complex TypeScript generics for generated content; prefer simple, maintainable types
Maintain type safety for dynamically loaded content by generating and referencing TypeScript definitions, with proper annotations for require() results
Do not use relative imports to generated artifacts; resolve via absolute node_modules paths or the package entry
Generated content is loaded at runtime, not build time; avoid static imports of generated modules
Prefer bracket notation for accessing slug-named properties with hyphens (e.g., prompts['slug-name'])
Avoid relative require('./generated/_index.js'); resolve absolute paths from process.cwd() into node_modules for generated assets
Files:
src/apis/api.tssrc/apis/vector.ts
🧬 Code graph analysis (2)
test/apis/api.test.ts (1)
src/apis/api.ts (1)
send(120-201)
src/apis/vector.ts (2)
src/router/data.ts (1)
json(155-165)src/router/response.ts (1)
json(55-69)
🔇 Additional comments (7)
CHANGELOG.md (1)
3-7: LGTM!The changelog entry accurately describes the fix for OpenTelemetry header propagation in outbound requests.
package.json (1)
3-3: LGTM!Version bump to 0.0.152 is correct for this patch-level fix.
src/apis/api.ts (2)
1-1: LGTM!Appropriate OpenTelemetry imports added to enable trace context propagation.
155-155: LGTM!The Authorization header is correctly set after trace injection to ensure it cannot be overridden by any previous headers or trace context injection.
test/apis/api.test.ts (3)
14-14: LGTM!Appropriate imports added for testing OpenTelemetry context behavior.
672-686: LGTM!Correctly uses
context.with(ROOT_CONTEXT, ...)to ensure no active trace context exists, validating that notraceparentheader is injected when there's no active span.
651-732: Ignore context isolation suggestion: tests already run in the default ROOT_CONTEXT and won’t inject trace headers when no active span exists—explicitcontext.with(ROOT_CONTEXT, …)is redundant.Likely an incorrect or invalid review comment.
|
|
||
| // inject trace context headers if there's an active context | ||
| const currentContext = context.active(); | ||
| propagation.inject(currentContext, headers, { | ||
| set: (carrier, key, value) => { | ||
| carrier[key] = value; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify trace header override behavior.
The trace context injection occurs after custom headers are copied (lines 142-144), which means propagation.inject will overwrite any traceparent or other trace headers passed in request.headers.
However, the test at lines 688-706 in test/apis/api.test.ts expects existing trace headers to be preserved. If there's an active OpenTelemetry context, the propagation.inject call will replace the custom traceparent header, potentially causing the test to fail in scenarios with an active trace context.
Run the following script to verify the test expectations:
🏁 Script executed:
#!/bin/bash
# Check if tests properly validate the interaction between custom traceparent headers and active context
rg -nP "should preserve existing trace headers" test/apis/api.test.ts -A 20Length of output: 724
🏁 Script executed:
#!/bin/bash
sed -n '130,180p' src/apis/api.tsLength of output: 1352
🏁 Script executed:
#!/bin/bash
rg -nP "import.*(context|propagation)" src/apis/api.tsLength of output: 60
Prevent trace header override
The injected setter must only add missing headers—otherwise propagation.inject will overwrite any custom traceparent. Change to:
propagation.inject(currentContext, headers, {
set: (carrier, key, value) => {
if (!(key in carrier)) {
carrier[key] = value;
}
},
});This ensures existing trace headers in request.headers are preserved.
🤖 Prompt for AI Agents
In src/apis/api.ts around lines 145 to 152, the propagation.inject setter
currently overwrites existing headers which can replace custom trace headers;
update the setter so it only assigns the value when the header key is not
already present in the carrier (i.e., check if key is absent before setting) to
preserve any existing trace headers in request.headers.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the test that was mentioned to see if it conflicts with the override behavior
sed -n '688,706p' test/apis/api.test.ts
Length of output: 557
@jhaynie, understood! The current implementation is correct for your use case—when there's an active OpenTelemetry context, it should override any manually passed trace headers.
However, I noticed that the test "should preserve existing trace headers passed in request" (lines 688-706 in test/apis/api.test.ts) might be misleading. It currently passes because there's no active OpenTelemetry context set up in that test, so propagation.inject doesn't inject anything, leaving the custom traceparent intact.
To properly test the override behavior, you might want to either:
- Update the test name and documentation to clarify it only preserves headers when there's no active context
- Add a separate test that verifies active context does override custom headers (using
context.with()to set up an active span)
This will make the test suite more explicit about the intended behavior.
🧬 Code Graph Analysis Results
[src/apis/api.ts] lines 1-14
- Complete snippet:
import { context, propagation } from '@opentelemetry/api';
import { getSDKVersion } from '../router/router';
import { isReadableStream } from '../types';
// allow the fetch function to be overridden
let apiFetch = globalThis.fetch;
// only used in tests
export function setFetch(f: typeof fetch) {
apiFetch = f;
}
export function getFetch() {
return apiFetch;
}Reason: This defines the override mechanism for fetch, which directly relates to the user’s note “we want it to override.”
✏️ Learnings added
Learnt from: jhaynie
PR: agentuity/sdk-js#197
File: src/apis/api.ts:145-152
Timestamp: 2025-10-07T02:33:29.185Z
Learning: In src/apis/api.ts, OpenTelemetry trace context propagation is intentionally designed to override any manually passed trace headers (like traceparent) when there's an active context. This ensures the active trace context takes precedence.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary by CodeRabbit